Add initial pinctrl support for NXP kinetis #39743

galak · 2021-10-26T17:38:53Z

No description provided.

Add initial version for NXP Kinetis based SoCs. Signed-off-by: Kumar Gala <[email protected]>

Convert driver to utilize new pinctrl API. Update devicetree binding to require pinctrl-0 and pinctrl-names properties. Signed-off-by: Kumar Gala <[email protected]>

Remove board specific calls to pinmux_pin_set() for UART as the UART driver will now handle the pin configuration via the pinctrl API. Signed-off-by: Kumar Gala <[email protected]>

jgediya · 2021-10-27T06:19:08Z

soc/arm/nxp_kinetis/common/pinctrl_soc.h

+	.mux = PORT_PCR_MUX(DT_PROP_BY_IDX(node_id, nxp_kinetis_port_pins, 1)) |		\
+		(DT_PROP(node_id, bias_pull_up) & (PORT_PCR_PE_MASK | PORT_PCR_PS_MASK)) |	\
+		(DT_PROP(node_id, bias_pull_down) & PORT_PCR_PS_MASK) |				\
+		(DT_PROP(node_id, drive_open_drain) & PORT_PCR_ODE_MASK), 			\


PORT_PCR_ODE_MASK is 0x20, If DT_PROP(node_id, drive_open_drain) return true(1) then '&' of them (1 & 0x20) will be 0. Instead we want 5th bit to be set here. Isn't it? Should we write here (DT_PROP(node_id, drive_open_drain) << PORT_PCR_ODE_SHIFT)? If this is true, then same can be applied to other '&' operations as well.

jgediya · 2021-10-27T06:19:25Z

soc/arm/nxp_kinetis/common/pinctrl_soc.h

+	.pin = DT_PROP_BY_IDX(node_id, nxp_kinetis_port_pins, 0),				\
+	.mux = PORT_PCR_MUX(DT_PROP_BY_IDX(node_id, nxp_kinetis_port_pins, 1)) |		\
+		(DT_PROP(node_id, bias_pull_up) & (PORT_PCR_PE_MASK | PORT_PCR_PS_MASK)) |	\
+		(DT_PROP(node_id, bias_pull_down) & PORT_PCR_PS_MASK) |				\


Shouldn't PORT_PCR_PE_MASK be used for pull-down?

Agreed. This should be (DT_PROP(node_id, bias_pull_down) & PORT_PCR_PE_MASK) |.

dts/bindings/serial/nxp,kinetis-uart.yaml

gmarull · 2021-10-27T08:40:23Z

soc/arm/nxp_kinetis/common/pinctrl_soc.h

+/**
+ * @file
+ * Public APIs for pin control drivers
+ */


can be removed

gmarull · 2021-10-27T08:40:54Z

soc/arm/nxp_kinetis/common/pinctrl_soc.h

+/**
+ * @brief Pin Controller Interface (NXP Kinetis)
+ * @defgroup pinctrl_interface_nxp_kinetis Pin Controller Interface
+ * @ingroup pinctrl_interface
+ * @{
+ */


if this file doesn't expose any public API this can likely be removed

gmarull · 2021-10-27T08:43:23Z

dts/bindings/serial/nxp,kinetis-uart.yaml

 compatible: "nxp,kinetis-uart"

-include: uart-controller.yaml
+include: [uart-controller.yaml, pinctrl-device.yaml]


suggestion: update binding description to mention pinctrl requirements, with some example.

jgediya · 2021-10-27T08:47:48Z

drivers/pinctrl/pinctrl_nxp_mcux.c

+			   uintptr_t reg)
+{
+	for (uint8_t i = 0U; i < pin_cnt; i++) {
+		pins[i].port_reg->PCR[pins[i].pin] = pins[i].mux;


There are some more valid bits in the PCR register, shouldn't this be read, clear lowest 16 bits, and '|' mux value operation? Maybe in some scenario, this can create problems?

Which bits would you preserve here? The idea is that all these bits are controlled by the pinctrl driver based on the devicetree properties.

IRQC bits, which are currently configured by gpio_mcux.c driver for interrupt configuration

henrikbrixandersen · 2021-10-27T18:44:29Z

drivers/pinctrl/pinctrl_nxp_mcux.c

+int pinctrl_configure_pins(const pinctrl_soc_pin_t *pins, uint8_t pin_cnt,
+			   uintptr_t reg)
+{
+	for (uint8_t i = 0U; i < pin_cnt; i++) {


Suggested change

for (uint8_t i = 0U; i < pin_cnt; i++) {

uint8_t i;

for (i = 0U; i < pin_cnt; i++) {

dts/bindings/serial/nxp,kinetis-uart.yaml

henrikbrixandersen · 2021-10-27T18:51:44Z

soc/arm/nxp_kinetis/common/pinctrl_soc.h

+	.pin = DT_PROP_BY_IDX(node_id, nxp_kinetis_port_pins, 0),				\
+	.mux = PORT_PCR_MUX(DT_PROP_BY_IDX(node_id, nxp_kinetis_port_pins, 1)) |		\
+		(DT_PROP(node_id, bias_pull_up) & (PORT_PCR_PE_MASK | PORT_PCR_PS_MASK)) |	\
+		(DT_PROP(node_id, bias_pull_down) & PORT_PCR_PS_MASK) |				\


Agreed. This should be (DT_PROP(node_id, bias_pull_down) & PORT_PCR_PE_MASK) |.

henrikbrixandersen · 2021-10-27T18:56:54Z

@galak Looks like a good starting point. We will likely need to add a custom drive strength dts property for selecting between high and low drive strength (the common drive-strength property expects a value in mA), but that can come at a later point in time.

I have opened a PR for setting the drive strength through the GPIO API here, but the drive strength applies to all digital outputs (e.g. high drive strength for PWM modulation of an LED): #39766

henrikbrixandersen · 2021-11-30T08:37:10Z

@galak What are your plans for getting this ready? If you do not have the time, I'd like to pick it up and get it ready for merging.

github-actions · 2022-01-30T00:29:08Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

dleach02 · 2022-02-01T19:24:48Z

@henrikbrixandersen, we are going to take this over as part of the larger transition of NXP parts to pinctrl. We have two alternative approaches for the RT platforms in #42238 and #41810 and plan on applying the adopted solution across the rest of the NXP platforms.

dleach02 · 2022-02-22T22:40:45Z

We are going to complete the Kinetis pintctrl change over with #43033. Many thanks to @galak for providing guidance.

galak added 3 commits October 26, 2021 12:37

drivers: pinctrl: nxp: initial version

97fc579

Add initial version for NXP Kinetis based SoCs. Signed-off-by: Kumar Gala <[email protected]>

drivers: serial: uart_mcux: use new pinctrl API

ef49c28

Convert driver to utilize new pinctrl API. Update devicetree binding to require pinctrl-0 and pinctrl-names properties. Signed-off-by: Kumar Gala <[email protected]>

boards: arm: nxp: use driver to set uart pins

83f10ec

Remove board specific calls to pinmux_pin_set() for UART as the UART driver will now handle the pin configuration via the pinctrl API. Signed-off-by: Kumar Gala <[email protected]>

galak requested review from MaureenHelm, dleach02, gmarull and mmahadevan108 as code owners October 26, 2021 17:38

galak requested a review from henrikbrixandersen October 26, 2021 17:38

github-actions bot added area: Boards area: Devicetree area: Devicetree Binding PR modifies or adds a Device Tree binding platform: NXP NXP labels Oct 26, 2021

jgediya reviewed Oct 27, 2021

View reviewed changes

dts/bindings/serial/nxp,kinetis-uart.yaml Show resolved Hide resolved

gmarull reviewed Oct 27, 2021

View reviewed changes

jgediya reviewed Oct 27, 2021

View reviewed changes

zephyrbot added area: Build System area: UART Universal Asynchronous Receiver-Transmitter labels Oct 27, 2021

zephyrbot requested review from dcpleung, mbolivar-nordic, nashif and tejlmand October 27, 2021 12:05

zephyrbot assigned dleach02 Oct 27, 2021

henrikbrixandersen requested changes Oct 27, 2021

View reviewed changes

carlescufi mentioned this pull request Oct 28, 2021

Road from pinmux to pinctrl #39740

Closed

29 tasks

dleach02 requested a review from danieldegrasse November 9, 2021 19:02

github-actions bot added the Stale label Jan 30, 2022

dleach02 removed the Stale label Feb 1, 2022

danieldegrasse mentioned this pull request Feb 21, 2022

Initial Kinetis pinctrl Implementation #43033

Merged

dleach02 closed this Feb 22, 2022

-	for (uint8_t i = 0U; i < pin_cnt; i++) {
+	uint8_t i;
+	for (i = 0U; i < pin_cnt; i++) {

Add initial pinctrl support for NXP kinetis #39743

Add initial pinctrl support for NXP kinetis #39743

Uh oh!

Conversation

galak commented Oct 26, 2021

Uh oh!

jgediya Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgediya Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgediya Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henrikbrixandersen commented Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henrikbrixandersen commented Nov 30, 2021

Uh oh!

github-actions bot commented Jan 30, 2022

Uh oh!

dleach02 commented Feb 1, 2022

Uh oh!

dleach02 commented Feb 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jgediya Oct 27, 2021 •

edited

Loading

jgediya Oct 27, 2021 •

edited

Loading

jgediya Oct 27, 2021 •

edited

Loading

henrikbrixandersen commented Oct 27, 2021 •

edited

Loading